Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled #23759

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Dec 19, 2024

Motivation

Modifications

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added type/bug The PR fixed a bug or issue reported a bug release/4.0.2 labels Dec 19, 2024
@poorbarcode poorbarcode self-assigned this Dec 19, 2024
@poorbarcode poorbarcode added this to the 4.1.0 milestone Dec 19, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 19, 2024
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 56.66667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 74.13%. Comparing base (bbc6224) to head (f2a2441).
Report is 815 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 31.57% 13 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23759      +/-   ##
============================================
+ Coverage     73.57%   74.13%   +0.56%     
+ Complexity    32624    31763     -861     
============================================
  Files          1877     1853      -24     
  Lines        139502   143391    +3889     
  Branches      15299    16279     +980     
============================================
+ Hits         102638   106308    +3670     
+ Misses        28908    28705     -203     
- Partials       7956     8378     +422     
Flag Coverage Δ
inttests 26.72% <43.33%> (+2.13%) ⬆️
systests 23.14% <43.33%> (-1.18%) ⬇️
unittests 73.67% <56.66%> (+0.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...apache/bookkeeper/mledger/ManagedLedgerConfig.java 96.42% <100.00%> (+0.13%) ⬆️
...pache/bookkeeper/mledger/impl/RangeSetWrapper.java 81.25% <100.00%> (-13.09%) ⬇️
...org/apache/pulsar/broker/ServiceConfiguration.java 98.14% <100.00%> (-1.25%) ⬇️
...rg/apache/pulsar/broker/service/BrokerService.java 84.87% <100.00%> (+4.09%) ⬆️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 79.30% <31.57%> (+<0.01%) ⬆️

... and 1017 files with indirect coverage changes

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, @poorbarcode . This is an essential improvement.

I'd like to request a change for the default value of managedLedgerPersistIndividualAckAsLongArray. It should be set to true in this PR. There's a long mailing list thread about this:
https://lists.apache.org/thread/kfm7n6xf9pf7h76k4gx83zv3c2kj6yy1

When we say there's support for downgrading (rollback), it doesn't
mean users don't need to take any action.

We ensure downgrade compatibility by providing an upgrade guide that
explains how to configure the system to allow rollbacks without losing
state information. If we didn't do this, we'd always be stuck in the
same situation whenever a new LTS version comes out.

Let's start making progress:

  • keep PR 9292 as default
  • implement PR 23759 without changing the PR 9292 default
  • write a proper upgrade guide for Pulsar 4.0 where rollback
    considerations are explained (separate PR)

@poorbarcode
Copy link
Contributor Author

@lhotari

Answered your question in the discussion channel: https://lists.apache.org/thread/kfm7n6xf9pf7h76k4gx83zv3c2kj6yy1

@poorbarcode
Copy link
Contributor Author

Hi Lari

Since there is no more arguments in the email list, I dismissed your change request

@poorbarcode poorbarcode dismissed lhotari’s stale review January 20, 2025 01:48

[DISCUSS] Add an optional config to disable the feature that compresses cursor metadata, which was contributed by #9292

@poorbarcode poorbarcode merged commit 4ee4633 into apache:master Jan 20, 2025
55 of 56 checks passed
@lhotari
Copy link
Member

lhotari commented Jan 20, 2025

Hi Lari

Since there is no more arguments in the email list, I dismissed your change request

@poorbarcode In Apache projects, we continue discussions until there's consensus on important decisions. We haven't reached consensus yet. Let's postpone cherry-picking to branch-4.0 until there's consensus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants